test: producer-to-collector routing on pg_stat_ch.block_format#117
test: producer-to-collector routing on pg_stat_ch.block_format#117JoshDreamland wants to merge 3 commits into
Conversation
Closes the producer-side contract gap: the unified Arrow exporter emits pg_stat_ch.block_format=arrow_events_raw on its OTLP envelope, while the legacy ArrowBatchBuilder path emits arrow_ipc. The central OTel collector's routingconnector is supposed to fan batches between the legacy query_logs_arrow target and the new events_raw target based on that marker. This test pins the producer half of the contract. New TAP test t/037_arrow_routing.pl exercises both arms: - arm 1 (unified=off, arrow_passthrough=on): legacy.jsonl grows, events_raw.jsonl and default.jsonl don't. - arm 2 (unified=on): events_raw.jsonl grows with arrow_events_raw in the routed payload, legacy.jsonl and default.jsonl don't. Stock otelcol-contrib image (matches existing t/024 setup, version 0.120.0). Routingconnector keyed on the resource attribute (where pg_stat_ch.block_format lives via PopulateResource), two file exporters writing JSONL to a bind-mounted dir the test reads from. No CH receiver — the events_raw column-set contract isn't validated here; that's covered by t/036 (producer Arrow IPC -> direct CH ingest) and by a separate receiver-faithful test in clickgres-platform. New helpers in t/psch.pm (psch_routing_collector_available, psch_start_routing_collector, psch_stop_routing_collector) live on a separate compose profile + container/ports so the new test can coexist with t/024's collector if both fire in the same suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ae80bf3 to
f87af5e
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new TAP test that validates the producer-side pg_stat_ch.block_format marker and an otelcol-contrib routingconnector “shape” by routing OTLP logs into different file exporters, ensuring legacy (arrow_ipc) vs unified (arrow_events_raw) batches fan out as expected during the Arrow exporter migration.
Changes:
- Introduces a new TAP test (
t/037_arrow_routing.pl) that runs legacy vs unified exporter arms and asserts which routed JSONL file grew. - Adds Docker Compose + collector config for a standalone routing collector using
routingconnectorandfileexporters. - Extends TAP helper module (
t/psch.pm) with start/stop/availability helpers for the routing collector.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| t/psch.pm | Adds helper functions to manage a dedicated routing-collector docker compose deployment for the new TAP test. |
| t/037_arrow_routing.pl | New TAP test that drives two producer configurations and asserts routing outcomes via file growth + marker grep. |
| docker/otel-routing/output/.gitignore | Keeps routed JSONL outputs out of git while preserving the directory. |
| docker/otel-routing/collector-config.yaml | Minimal collector pipeline using routingconnector and per-route file exporters for assertions. |
| docker/docker-compose.arrow-route.yml | Standalone compose stack for the routing collector with distinct ports/container name from existing otelcol test stack. |
Comments suppressed due to low confidence (1)
t/037_arrow_routing.pl:178
- With cleanup handled via an END block (see earlier comment), the unconditional stop call here should be removed so the test doesn’t tear down a collector it didn’t start.
psch_stop_routing_collector();
done_testing();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t/013_clickhouse_tls.pl's reconnect subtest polled for the resumed export within 30s of restarting the ClickHouse container. The exporter's reconnect backoff (stats_exporter.cc PschGetRetryDelayMs) is exponential — base 1s, doubling per consecutive failure, capped at 60s. Failed flush attempts start accumulating as soon as CH goes down, well before the container finishes restarting, and the cumulative wait before the Nth retry is the sum of all prior delays: 1, 3, 7, 15, 31, 63, 123s. A restart slow enough to rack up 5-6 failures — plausible under a loaded CI runner — pushes the bgworker's next attempt past the 30s mark even though ClickHouse itself came back much sooner. Observed exactly this on CI twice in a row while rebasing PR #117 onto post-#116 main: identical assertion, identical 'got 0' failure, while the same commit had passed cleanly on a less-loaded run days earlier. Not a regression — the backoff constants and this test's timeout have coexisted; it just needed a slow enough runner to surface. Bump to 90s, comfortably past the 6-failure (63s) cumulative-backoff case plus round-trip margin. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
t/037_arrow_routing.pl:26
- The header comment describes three test arms (unified off, then off+passthrough, then unified on), but the script actually runs only two arms and both set otel_arrow_passthrough=on. This mismatch can confuse future maintainers when updating the test.
# 2. Start a node configured to ship to it; first with the unified GUC OFF,
# then OFF + with otel_arrow_passthrough so the legacy ArrowBatchBuilder
# path fires; finally with the unified GUC ON.
t/037_arrow_routing.pl:46
- The test always tears down the routing collector at the end, even if it was already running before the test started. This can disrupt local dev workflows or parallel runs. It’s also safer to register teardown in an END block so cleanup still happens if the test exits early.
eval { psch_start_routing_collector() };
if ($@) {
plan skip_all => "Docker / routing collector not available: $@";
}
}
t/037_arrow_routing.pl:177
- With teardown handled conditionally in an END block, this unconditional shutdown is redundant and will still bring down a pre-existing collector when the test didn’t start it.
psch_stop_routing_collector();
Addresses review feedback (1 Copilot + 4 Cursor comments) on t/037_arrow_routing.pl, verified live via prove against a real build (not just code review): - Track whether this run started the routing collector ($started_routing_collector, set before the eval'd start attempt so a partial startup still counts). Only tear it down if so — a developer's manually-started collector, or one left behind by a crashed prior run, is left alone. - Move teardown into an END block (with 'local $?' to stop system() inside END from clobbering the script's exit code) so cleanup runs on every exit path: normal completion, plan skip_all, or a die() anywhere in the test body. Previously a die (e.g. the open-or-die below) skipped cleanup entirely, leaking the container and its fixed host ports (14317, 23133) -- ports t/026_arrow_dump.pl and t/036_unified_arrow_e2e.pl rely on being unbound. - Replace the bare 'open ... or die' on arm 2's spot-check with a Test::More SKIP block gated on whether the file actually grew, so a growth timeout produces a clean failed assertion instead of aborting the script mid-run. - Add per-arm producer-side bookkeeping (psch_wait_for_export + psch_get_stats asserting exported/send_failures), matching the existing t/024_otel_export.pl idiom, before the JSONL-growth checks. On failure this distinguishes 'producer never exported' from 'producer exported but routing/collector dropped it'. - Document the fixed-host-port single-instance-per-machine constraint in the header rather than parameterizing container/output naming: CI runs the TAP suite sequentially (prove without -j, single TAP job), and parameterizing names without also parameterizing the ports (14317/23133) wouldn't actually fix concurrent local runs while risking t/026 and t/036's assumption that those ports are unbound. Live-run verification surfaced a genuine pre-existing race unrelated to the above: the otelcol fileexporter writes a JSON line and its trailing newline as separate writes, and arm 1's node can flush trailing batches right up through stop(). If arm 2 snapshots its baselines mid-write, those late bytes get mis-attributed. Added wait_for_quiet() (polls combined JSONL size across all three files until stable for 1.5s, 15s cap) before arm 2's baselines to close the window. Confirmed via 3 live prove runs against a real build: one initial 11/12 run caught the race, then 12/12 with the collector auto-started (and torn down after), and 12/12 with the collector pre-started manually (and correctly left running after). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| system("docker compose -f $compose_file up -d") == 0 | ||
| or die "Failed to start routing collector container"; | ||
|
|
||
| for my $i (1..30) { | ||
| my $result = `curl -sf 'http://localhost:23133/' 2>/dev/null`; | ||
| return 1 if $result =~ /Server available/; | ||
| sleep(1); | ||
| } | ||
| die "Routing collector container failed to become healthy"; |
| # Flow: | ||
| # 1. Spin up the routing collector via docker compose (otel-routing profile). | ||
| # 2. Start a node configured to ship to it, two arms: |
| # Let arm 1's trailing collector writes settle before snapshotting this | ||
| # arm's baselines (see wait_for_quiet). | ||
| wait_for_quiet(1.5, 15, | ||
| map { "$output_dir/$_.jsonl" } qw(legacy events_raw default)); |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.
| # Let arm 1's trailing collector writes settle before snapshotting this | ||
| # arm's baselines (see wait_for_quiet). | ||
| wait_for_quiet(1.5, 15, | ||
| map { "$output_dir/$_.jsonl" } qw(legacy events_raw default)); |
There was a problem hiding this comment.
Quiet wait failure ignored
Medium Severity
Arm 2 calls wait_for_quiet to settle file writes but ignores its return value. If the timeout is reached and files are still growing, baselines are taken prematurely. This can lead to flaky assertions, mis-attributed bytes, or incorrect verification of routing isolation.
Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.
|
|
||
| my $default_after = current_size("$output_dir/default.jsonl"); | ||
| is($default_after, $default_before, | ||
| 'arm 1 (arrow_ipc): default.jsonl did NOT grow (no unmatched batches)'); |
There was a problem hiding this comment.
Isolation checks race async writes
Medium Severity
After wait_for_growth on the expected JSONL file, each arm immediately asserts the other sinks did not grow. The test notes the file exporter writes asynchronously, but there is no quiescence wait before those cross-file size checks, so a late write can yield a false pass on routing isolation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 71ecff9. Configure here.


Summary
Closes the producer-side half of the Arrow exporter migration contract: pin
pg_stat_ch.block_formatemission and the routing-connector match-shape via a generic TAP test.The unified Arrow exporter (PR #116's commit 4) emits
pg_stat_ch.block_format=arrow_events_rawon its OTLP envelope; the legacyArrowBatchBuilderpath emitsarrow_ipc. In prod, the centraldatagres-otelcoldeployment'sroutingconnectoris supposed to fan batches between the legacyquery_logs_arrowreceiver target and a newevents_rawreceiver target based on that marker. This test pins the producer side of that contract: it doesn't run a real receiver (no CH, nodatagres-arrow-exporter) — it runs stockotelcol-contribwith twofileexporters and asserts which one received which arm's batches.What it catches (and what it doesn't)
Catches:
pg_stat_ch.block_formatemission (e.g. unified path silently falling back toarrow_ipc).Does NOT catch:
events_rawcolumn-set contract or the receiver's wire-format expectations — those live behinddatagres-arrow-exporterinclickgres-platformand are covered by a separate receiver-faithful test there (in flight).t/036_unified_arrow_e2e.plcontinues to handle the producer Arrow IPC -> direct CH ingest path.Mechanics
docker/docker-compose.arrow-route.yml(separate from the existingdocker-compose.otel.ymlsot/024andt/037can coexist; different ports, different container name).docker/otel-routing/collector-config.yaml— stockotel/opentelemetry-collector-contrib:0.120.0,routingconnectorkeyed on the resource attribute, threefileexporters (legacy / events_raw / default fall-through) writing JSONL to a bind-mounted dir.t/psch.pmhelpers:psch_routing_collector_available/psch_start_routing_collector/psch_stop_routing_collector.t/037_arrow_routing.plruns two arms (legacy + unified) and asserts that the corresponding file grew while the other files didn't. The unified arm also greps the routed JSONL tail forarrow_events_rawto pin the marker value content.Stacking
Based on
unified_arrow_exporter(PR #116). Hard-depends on commits 2 (the GUC) and 4 (the marker swap). Will retarget tomainafter #116 merges.Test plan
unified_arrow_exporter.psch_*_otelcol).🤖 Generated with Claude Code